Add try/ref versions to unwrap #206

rassvet2 · 2022-10-08T21:02:15Z

I extended Unwrap to add one that returns Option, one that returns reference, and a combination of the two, for my use.
It's ready to merge, but I don't think it's well discussed and considered. try_unwrap_<variant>_ref is... little uncool.
So I'll leave it as a draft to solicit opinions.

#[derive(Unwrap)]
enum Number {
    Integer(i64),
    Decimal(f64),
    #[unwrap(ref)] // this affects only `FromString`
    FromString(String),
}

#[derive(Unwrap)]
#[unwrap(ref)] // this affects the entire enum,
               // and adds `unwrap_<name>_ref` and `try_unwrap_<name>_ref`.
enum Maybe<T> {
    Just(T),
    Nothing,
}

fn main() {
    assert_eq!(Maybe::Just(1).unwrap_just(), 1);
    assert_eq!(Maybe::Just(2).try_unwrap_just(), Some(2));
    assert_eq!(Maybe::Nothing.try_unwrap_just(), None);
    assert_eq!(*(&Maybe::Just(42)).unwrap_just_ref(), 42);
}

This relates to #186.
var_a looks better than try_unwrap_var_a_ref. But I think in that case it probably should not be implemented in Unwrap.
Initially, I added it in Unwrap because it was tedious, but maybe it should be split into TryUnwrap, UnwrapRef, and TryUnwrapRef.

Is there anything else I have to do to be merged?

rassvet2 · 2022-10-09T03:11:31Z

There is a known issue with *_ref naming conflicts when an enum has SomeVariant and SomeVariantRef.
I need advice on this as well.

JelteF · 2022-10-15T22:53:45Z

I didn't look at the code in detail, but a few thoughts on the general design:

For completeness this should also add unwrap_xxx_mut versions (unless somehow hard to do)
I really don't like the try_unwrap prefix. Some other options I can think of: as_thing, try_into_thing, try_thing. I think I like try_into_thing the best. (I will continue calling it try_unwrap in the rest of this comment.
Should try_unwrap return an Error instead of an Option? I think that would probably be better. If so, this issue is quite relevant: Make TryInto use better error type #173
If we rename try_unwrap to try_into I feel like we should use something different than the Unwrap derive? Maybe TryIntoVariant

This seems rare enough that i wouldn't consider it a problem:

There is a known issue with *_ref naming conflicts when an enum has SomeVariant and SomeVariantRef.

JelteF · 2022-10-15T22:57:47Z

To clarify: The most open questions and discussion points for me are around the try_unwrap version of this derive. I think it makes sense to have the unwrap_thing_ref and unwrap_thing_mut without any further discussion on the design. So I think it makes sense to separete these two things in separate PRs.

rassvet2 · 2022-10-19T13:05:13Z

for 1 and 2:

Yes, I will. I like the shortness of as, but it seems misleading. Looking at std, it seems that functions starting with as_ rarely return Option or Result. So I'm going to use try_into.

for 3 and 4:

Makes sense. I just forgot that TryInto returns Result, not Option. I will make it returns the original value as an error for now.
As for try_unwrap, I'm going to delete it temporarily and separate it into a new PR.

JelteF · 2023-03-18T08:52:04Z

@rassvet2 are you still planning on making your suggested changes?

rassvet2 · 2023-03-23T06:36:32Z

I'm sorry I left this hanging for so long.
I will work on this. It may take about two weeks.

rassvet2 · 2023-03-29T23:21:23Z

It's ready for review. Finally, I decided like:

try_unwrap_* was renamed to try_into_* and moved to TryIntoVariant (Isn't it a bit long compared to Unwrap?)
Both Unwrap and TryIntoVariant basically generate owned versions only, but ref and ref_mut can also be generated by specifying them in the attribute.
try_into_* returns a Result value. This will be Ok if the conversion succeeds, and Err if it fails. Ok contains the converted value, and Err contains the original value.
This makes it useful when you do not want to lose ownership if conversion fails.

doc/unwrap.md

src/try_into_variant.rs

JelteF

Overall this looks good! But it needs a rebase with the current master branch. Quite a few things have changed in the file organization of the library since this PR was originally opened.

rassvet2 · 2023-04-07T08:48:58Z

I'm wondering again if I should rename TryIntoVariant. Now I don't feel it's a good name. Since these functions don't convert one enum variant into another, they're not a conversion like std::convert. In that respect, Unwrap was a good name as it "unwraps" an enum variant into a tuple of its fields.
However, I can't think of anything that fits. Do you agree with TryIntoVariant, or not?

JelteF · 2023-04-08T09:30:15Z

Since these functions don't convert one enum variant into another, they're not a conversion like std::convert

That's a really good point. I think you're right that TryUnwrap is probably the best name. Both Rc and Arc have a try_unwrap method too that does exactly this, so there's also prior art from std on this naming. I'm sorry for the back and forth on the naming.

src/convert.rs

impl/doc/try_unwrap.md

tests/try_unwrap.rs

JelteF

Looks pretty good overal, just some small suggestions. Also there's a small merge conflict in the CHANGELOG.md now that #256 is merged which cleaned it up a bit.

Co-authored-by: Jelte Fennema <github-tech@jeltef.nl>

JelteF mentioned this pull request Dec 29, 2022

Feature request: More functions for the Unwrap derive? #227

Open

rassvet2 marked this pull request as ready for review March 29, 2023 22:33

JelteF reviewed Apr 3, 2023

View reviewed changes

doc/unwrap.md Outdated Show resolved Hide resolved

JelteF reviewed Apr 3, 2023

View reviewed changes

src/try_into_variant.rs Outdated Show resolved Hide resolved

JelteF requested changes Apr 3, 2023

View reviewed changes

rassvet2 force-pushed the unwrap-try-ref branch from 805f4dc to c87c3eb Compare April 6, 2023 22:50

rassvet2 requested a review from JelteF April 7, 2023 08:23

JelteF reviewed Apr 8, 2023

View reviewed changes

src/convert.rs Outdated Show resolved Hide resolved

rassvet2 changed the title ~~add try/ref versions to unwrap~~ Add try/ref versions to unwrap Apr 8, 2023

rassvet2 requested a review from JelteF April 8, 2023 19:27

JelteF reviewed Apr 8, 2023

View reviewed changes

impl/doc/try_unwrap.md Outdated Show resolved Hide resolved

JelteF reviewed Apr 8, 2023

View reviewed changes

tests/try_unwrap.rs Show resolved Hide resolved

JelteF previously approved these changes Apr 8, 2023

View reviewed changes

rassvet2 dismissed JelteF’s stale review via fd8bbfb April 8, 2023 20:29

rassvet2 and others added 7 commits April 9, 2023 05:49

add try/ref versions to unwrap

7af1abb

fix error message and format code

0bcf752

split try versions and add mut version

9807fc1

add inline attr, format codes

bb4a6eb

update docs and tests

1174241

remove instant deref from the example

8a36784

Co-authored-by: Jelte Fennema <github-tech@jeltef.nl>

add dedicated error type for TryIntoVariant

bc7c243

rassvet2 added 5 commits April 9, 2023 05:49

move TryIntoError into a dedicated file

9df3a8f

fix test and doc errors

49e0e9b

rename TryIntoVariant to TryUnwrap

7605b92

update CHANGELOD.md

f72be88

update tests for try_unwrap

0cfce66

rassvet2 force-pushed the unwrap-try-ref branch from fd8bbfb to 0cfce66 Compare April 8, 2023 21:10

update try_unwrap.md

b9ab6d4

rassvet2 requested a review from JelteF April 8, 2023 21:21

add tests for mutable reference

11e2061

JelteF approved these changes Apr 10, 2023

View reviewed changes

JelteF enabled auto-merge (squash) April 10, 2023 12:14

JelteF merged commit 70c43f9 into JelteF:master Apr 10, 2023

tyranron added this to the 1.0.0 milestone May 15, 2023

tyranron assigned rassvet2 May 15, 2023

tyranron linked an issue Dec 22, 2023 that may be closed by this pull request

Complete enum derives with getters which return Option<&T> #186

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add try/ref versions to unwrap #206

Add try/ref versions to unwrap #206

rassvet2 commented Oct 8, 2022

rassvet2 commented Oct 9, 2022

JelteF commented Oct 15, 2022 •

edited

Loading

JelteF commented Oct 15, 2022 •

edited

Loading

rassvet2 commented Oct 19, 2022

JelteF commented Mar 18, 2023

rassvet2 commented Mar 23, 2023

rassvet2 commented Mar 29, 2023

JelteF left a comment •

edited

Loading

rassvet2 commented Apr 7, 2023

JelteF commented Apr 8, 2023

JelteF left a comment

Add try/ref versions to unwrap #206

Add try/ref versions to unwrap #206

Conversation

rassvet2 commented Oct 8, 2022

rassvet2 commented Oct 9, 2022

JelteF commented Oct 15, 2022 • edited Loading

JelteF commented Oct 15, 2022 • edited Loading

rassvet2 commented Oct 19, 2022

JelteF commented Mar 18, 2023

rassvet2 commented Mar 23, 2023

rassvet2 commented Mar 29, 2023

JelteF left a comment • edited Loading

Choose a reason for hiding this comment

rassvet2 commented Apr 7, 2023

JelteF commented Apr 8, 2023

JelteF left a comment

Choose a reason for hiding this comment

JelteF commented Oct 15, 2022 •

edited

Loading

JelteF commented Oct 15, 2022 •

edited

Loading

JelteF left a comment •

edited

Loading